Add decoded span assert rules - #12088
Conversation
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 5134699 · Any feedback? Reach out in #deveng-pr-agent |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please try again by commenting @autotest review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513469965b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 798798a | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
5134699 to
d7f5363
Compare
|
@autotest review |
There was a problem hiding this comment.
Unordered trace matching can reject a valid trace when a reusable childOfPrevious matcher first probes a non-matching candidate: assertSpan mutates the matcher to that candidate's parent ID, so later candidates are checked against stale state. Keep the resolved parent matcher local instead of mutating the configured matcher.
🤖 Datadog Autotest · Commit d7f5363 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
d7f5363 to
798798a
Compare
There was a problem hiding this comment.
More details
The decoder and assertion paths do not show a confirmed behavioral regression or diff-only production hazard. Adversarial cases were reviewed for malformed JSON, unsigned IDs, missing fields, duplicate/unordered trace matching, ancestry ordering, partial trace chunks, and nested meta_struct values; execution was blocked before tests ran because the checkout requires JDK 25 and the sandbox has only JDK 8/11/17/21.
🤖 Datadog Autotest · Commit 798798a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 798798ab44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Json(name = "meta_struct") | ||
| Map<String, Object> metaStruct; | ||
|
|
||
| Map<String, Double> metrics; |
There was a problem hiding this comment.
When the same assertion runs against the in-process v0.4 decoder and the /test/traces JSON decoder, integral metrics are Integer or Long in SpanV04.unpackNumber, but this declaration forces every JSON metric to Double. Consequently, an assertion such as metric("_dd.top_level", is(1)) passes against one backend and fails against the other, while integer values above 2^53 are rounded outright. Preserve integral JSON numbers or make metric matching compare numeric values rather than wrapper types.
Useful? React with 👍 / 👎.
| public SpanMatcher childOfIndex(int parentSpanIndex) { | ||
| this.parentIdMatcher = null; | ||
| this.parentSpanIndex = parentSpanIndex; |
There was a problem hiding this comment.
When a computed parent index is negative, childOfIndex(-1) clears parentIdMatcher and stores the negative index; parentIdMatcher(...) then skips indexed lookup because it only handles values >= 0 and returns the cleared matcher. The parent assertion is therefore silently disabled, allowing a span with an unrelated parent to pass. Reject negative indexes when configuring the matcher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah... I'll fix it
| public SpanMatcher type(String type) { | ||
| this.typeMatcher = is(type); | ||
| return this; |
There was a problem hiding this comment.
Serialized v0.4 spans can legitimately have a null type, but calling the documented type(null) path constructs Matchers.is(null). That matcher's test method invokes expected.equals(...), so the assertion throws a NullPointerException even when the decoded type is also null instead of reporting a match. Select isNull() when type is null or provide a matcher-based overload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's missing the isNull() default matcher... AI failed to copy it 😮💨
I'll fix it.
| for (List<SpanJson> spans : rawTraces) { | ||
| List<DecodedSpan> decodedSpans = spans == null ? emptyList() : new ArrayList<>(spans); | ||
| traces.add(new TraceJson(decodedSpans)); |
There was a problem hiding this comment.
When a well-formed JSON span object is missing trace_id or span_id—for example after an endpoint schema mismatch or a truncated response—Moshi initializes those fields to null, the getters convert them to zero, and this conversion accepts the object without validation. A partial matcher such as span().root() can therefore pass against an invalid decoded span rather than exposing the malformed test-agent response. Validate required span fields before wrapping each SpanJson.
Useful? React with 👍 / 👎.
1 participant
What Does This Do
This PR introduces decoded span assert rules.
EDIT: This PR unit tests need test agent JSON span deserialization first. I’ll introduce an intermediate PR to the stack to add it. It is still good to review in the meantime.
Motivation
The goal is to provide capabilities for smoke tests to evaluate trace and span structures and values.
Additional Notes
This is mainly inspired from my prior work with instrumentation tests but decoded spans are not like DDSpan. They don’t hold the same values / types so new rules needed to be put in place for smoke tests.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMLP-1247